Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(qns): fallback if server is not sending NEW_TOKEN #1837

Merged
merged 7 commits into from
Apr 23, 2024
Merged

fix(qns): fallback if server is not sending NEW_TOKEN #1837

merged 7 commits into from
Apr 23, 2024

Conversation

mxinden
Copy link
Collaborator

@mxinden mxinden commented Apr 17, 2024 •
edited
Loading

#1676 ensured for the client to wait for a resumption token from the server if one is needed, that is when running the resumption QUIC Interop testcase. #1676 accidentally removed the fallback using a session ticked if no resumption token was available.

https://github.com/mozilla/neqo/pull/1676/files#diff-64d071700a607884926de1b2caaf1588558fceda6d3a0e899cfb3c3b7e5ae41fL1353-L1359

This commit re-introduces the fallback and enables the resumption QUIC Interop testcase on CI.

Along the way, it simplifies Runner::run and reserves the resume command line argument for the QUIC Interop testcases only. Let me know if this adds too much noise to the actual fix. Happy to extract it into a separate pull request.

Test failure reported in #1786 (comment).

@mxinden
feat(qns): add resumption testcase
2935e59 
Test failure reported in mozilla#1786 (comment).

Signed-off-by: Max Inden <[email protected]>
@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 17, 2024 •
edited
Loading

Benchmark results

Performance differences relative to 4fc4d16.

  • drain a timer quickly time: [430.85 ns 437.41 ns 443.50 ns]
    change: [-0.7742% +0.5155% +1.8832%] (p = 0.43 > 0.05)
    No change in performance detected.

  • coalesce_acked_from_zero 1+1 entries
    time: [202.60 ns 203.08 ns 203.55 ns]
    change: [+2.2678% +2.6124% +2.9801%] (p = 0.00 < 0.05)
    💔 Performance has regressed.

  • coalesce_acked_from_zero 3+1 entries
    time: [246.26 ns 246.90 ns 247.59 ns]
    change: [+2.9003% +3.4231% +3.8724%] (p = 0.00 < 0.05)
    💔 Performance has regressed.

  • coalesce_acked_from_zero 10+1 entries
    time: [245.97 ns 251.01 ns 260.31 ns]
    change: [+3.1883% +4.1610% +5.5344%] (p = 0.00 < 0.05)
    💔 Performance has regressed.

  • coalesce_acked_from_zero 1000+1 entries
    time: [225.85 ns 226.04 ns 226.25 ns]
    change: [+2.4067% +3.0924% +3.7850%] (p = 0.00 < 0.05)
    💔 Performance has regressed.

  • RxStreamOrderer::inbound_frame()
    time: [118.44 ms 118.63 ms 118.90 ms]
    change: [-1.0824% -0.8312% -0.5863%] (p = 0.00 < 0.05)
    Change within noise threshold.

  • transfer/Run multiple transfers with varying seeds
    time: [125.00 ms 125.30 ms 125.61 ms]
    thrpt: [31.845 MiB/s 31.922 MiB/s 32.000 MiB/s]
    change:
    time: [+0.2035% +0.5363% +0.8800%] (p = 0.00 < 0.05)
    thrpt: [-0.8723% -0.5334% -0.2031%]
    Change within noise threshold.

  • transfer/Run multiple transfers with the same seed
    time: [125.29 ms 125.52 ms 125.76 ms]
    thrpt: [31.806 MiB/s 31.868 MiB/s 31.927 MiB/s]
    change:
    time: [-0.0022% +0.2528% +0.4999%] (p = 0.05 < 0.05)
    thrpt: [-0.4974% -0.2521% +0.0022%]
    Change within noise threshold.

  • 1-conn/1-100mb-resp (aka. Download)/client
    time: [1.1342 s 1.1404 s 1.1479 s]
    thrpt: [87.115 MiB/s 87.690 MiB/s 88.170 MiB/s]
    change:
    time: [+3.8486% +4.5461% +5.3651%] (p = 0.00 < 0.05)
    thrpt: [-5.0919% -4.3484% -3.7060%]
    💔 Performance has regressed.

  • 1-conn/10_000-parallel-1b-resp (aka. RPS)/client
    time: [424.79 ms 427.13 ms 429.48 ms]
    thrpt: [23.284 Kelem/s 23.412 Kelem/s 23.541 Kelem/s]
    change:
    time: [-1.1971% -0.4592% +0.2963%] (p = 0.23 > 0.05)
    thrpt: [-0.2955% +0.4613% +1.2116%]
    No change in performance detected.

  • 1-conn/1-1b-resp (aka. HPS)/client
    time: [48.905 ms 49.192 ms 49.451 ms]
    thrpt: [20.222 elem/s 20.328 elem/s 20.448 elem/s]
    change:
    time: [-3.6597% -2.8579% -2.1051%] (p = 0.00 < 0.05)
    thrpt: [+2.1504% +2.9420% +3.7987%]
    💚 Performance has improved.

Client/server transfer results

Transfer of 134217728 bytes over loopback.

Client Server CC Pacing Mean [ms] Min [ms] Max [ms] Relative
msquic msquic 760.4 ± 280.8 404.1 1169.4 1.00
neqo msquic reno on 1007.7 ± 250.8 754.4 1447.9 1.00
neqo msquic reno 877.5 ± 167.7 747.3 1300.2 1.00
neqo msquic cubic on 875.9 ± 185.7 760.6 1370.5 1.00
neqo msquic cubic 1030.4 ± 202.8 779.3 1460.3 1.00
msquic neqo reno on 4440.8 ± 215.2 4185.3 4927.8 1.00
msquic neqo reno 4547.7 ± 363.1 4147.0 5152.7 1.00
msquic neqo cubic on 4554.1 ± 329.7 4240.0 5388.0 1.00
msquic neqo cubic 4512.0 ± 276.0 4172.5 5045.0 1.00
neqo neqo reno on 3485.3 ± 153.7 3238.6 3784.6 1.00
neqo neqo reno 3934.2 ± 401.0 3075.1 4482.2 1.00
neqo neqo cubic on 4444.6 ± 234.5 4218.7 4878.3 1.00
neqo neqo cubic 4414.2 ± 246.2 4002.8 4749.6 1.00

⬇️ Download logs

@mxinden
Copy link
Collaborator Author

mxinden commented Apr 18, 2024

Currently debugging neqo -> msquic resumption test failure.

@mxinden
Copy link
Collaborator Author

mxinden commented Apr 18, 2024

Got it. I introduced this regression in #1676. While #1676 fixes the race condition around receiving the resumption token from the server, it removes the fallback using a session ticked 🤦.

https://github.com/mozilla/neqo/pull/1676/files#diff-64d071700a607884926de1b2caaf1588558fceda6d3a0e899cfb3c3b7e5ae41fL1353-L1359

Yet another good reason for #1785 or #1847.

Will push a patch in a bit.

//CC @larseggert

@mxinden mxinden changed the title feat(qns): add resumption testcase fix(qns): fallback if server not sending NEW_TOKEN Apr 18, 2024
@mxinden mxinden changed the title fix(qns): fallback if server not sending NEW_TOKEN fix(qns): fallback if server is not sending NEW_TOKEN Apr 18, 2024
@codecov Codecov
Copy link

codecov bot commented Apr 18, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.23%. Comparing base (4fc4d16) to head (710c500).

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #1837   +/-   ##
=======================================
  Coverage   93.23%   93.23%           
=======================================
  Files         110      110           
  Lines       35749    35749           
=======================================
+ Hits        33329    33330    +1     
+ Misses       2420     2419    -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mxinden mxinden marked this pull request as ready for review April 18, 2024 18:11
@larseggert larseggert added this pull request to the merge queue Apr 23, 2024
Merged via the queue into mozilla:main with commit da9766e Apr 23, 2024
14 checks passed
@goatgoose goatgoose mentioned this pull request Apr 26, 2024
Open
@mxinden mxinden mentioned this pull request May 2, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martinthomson martinthomson martinthomson approved these changes

@KershawChang KershawChang Awaiting requested review from KershawChang KershawChang is a code owner

@larseggert larseggert Awaiting requested review from larseggert larseggert is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mxinden @martinthomson @larseggert